feat!: move to new wallpaper api #17
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WallpapersSkill
User->>WallpapersSkill: Request change wallpaper
WallpapersSkill->>WallpapersSkill: fetch_wallpapers()
WallpapersSkill-->>User: Announce wallpaper change
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
dont try to set the wallpaper in the skill, just emit the proper event
JarbasAl
force-pushed
the
fix_plugin_compat
branch
from
2d5c7db
to
6a78e08
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
__init__.py(5 hunks)
🔇 Additional comments (2)
__init__.py (2)
8-8: LGTM: Clean import and comment fix
The import of wallpaper search functions and the comment correction look good. The initialization code maintains proper settings management while removing direct wallpaper handling.
Also applies to: 20-20
233-234: LGTM: Proper PHAL integration
The implementation correctly delegates wallpaper management to PHAL by:
- Registering as a wallpaper provider
- Implementing proper message handlers
- Using the correct message bus events
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
__init__.py (1)
Line range hint
165-184: Consider enhancing PHAL integration with response handlingThe PHAL integration is good but could be improved by handling responses from the wallpaper manager. Consider subscribing to
ovos.wallpaper.manager.set.wallpaper.responseevents in theregister_with_PHALmethod to track the success/failure of wallpaper changes.Add this to the
register_with_PHALmethod:def register_with_PHAL(self): self.bus.emit(Message("ovos.wallpaper.manager.register.provider", {"provider_name": self.skill_id, "provider_display_name": self.name})) self.bus.on(f"{self.skill_id}.get.wallpaper.collection", self.handle_wallpaper_scan) self.bus.on(f"{self.skill_id}.get.new.wallpaper", self.handle_wallpaper_get) + self.bus.on("ovos.wallpaper.manager.set.wallpaper.response", self._handle_wallpaper_response) wallpapers = list(self.iter_wallpapers()) self.bus.emit(Message("ovos.wallpaper.manager.collect.collection.response", {"provider_name": self.skill_id, "wallpaper_collection": wallpapers})) +def _handle_wallpaper_response(self, message): + success = message.data.get("success", False) + if not success: + error = message.data.get("error", "unknown error") + self.log.error(f"Failed to set wallpaper: {error}") + self.speak_dialog("wallpaper.change.failed")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
__init__.py(5 hunks)
🔇 Additional comments (1)
__init__.py (1)
8-8: LGTM: Import statement is appropriate
The import of latest_reddit from wallpaper_changer.search is correctly used throughout the code for fetching wallpapers.
JarbasAl
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
__init__.py (3)
5-5: Remove unused importThe
resting_screen_handlerdecorator is imported but never used in the code.-from ovos_workshop.decorators import intent_handler, resting_screen_handler +from ovos_workshop.decorators import intent_handler🧰 Tools
🪛 Ruff
5-5:
ovos_workshop.decorators.resting_screen_handlerimported but unused(F401)
27-28: Remove unnecessary f-stringsThe strings don't contain any placeholders, making the f-string prefix unnecessary.
- self.gui.register_handler(f'wallpaper.next', self.handle_next) - self.gui.register_handler(f'wallpaper.prev', self.handle_prev) + self.gui.register_handler('wallpaper.next', self.handle_next) + self.gui.register_handler('wallpaper.prev', self.handle_prev)🧰 Tools
🪛 Ruff
27-27: f-string without any placeholders
Remove extraneous
fprefix(F541)
28-28: f-string without any placeholders
Remove extraneous
fprefix(F541)
Line range hint
71-128: Add error handling for empty picture lists and index boundsThe intent handlers need proper error handling for cases where:
- The picture list is empty
- The index is out of bounds
- The API request fails
Example implementation for one handler (apply similar pattern to others):
@intent_handler("wallpaper.random.intent") def handle_random_wallpaper(self, message): - image = self.fetch_wallpapers() - self.change_wallpaper(image) - self.speak_dialog("wallpaper.changed") + try: + image = self.fetch_wallpapers() + if not image: + self.speak_dialog("error.fetching.wallpaper") + return + self.change_wallpaper(image) + self.speak_dialog("wallpaper.changed") + except Exception as e: + LOG.exception(f"Error in handle_random_wallpaper: {str(e)}") + self.speak_dialog("error.fetching.wallpaper")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
logo.pngis excluded by!**/*.png
📒 Files selected for processing (4)
README.md(1 hunks)__init__.py(4 hunks)gui/qt5/slideshow.qml(0 hunks)requirements.txt(0 hunks)
💤 Files with no reviewable changes (2)
- gui/qt5/slideshow.qml
- requirements.txt
🧰 Additional context used
🪛 Markdownlint
README.md
3-3: null
Bare URL used
(MD034, no-bare-urls)
🪛 Ruff
__init__.py
5-5: ovos_workshop.decorators.resting_screen_handler imported but unused
(F401)
27-27: f-string without any placeholders
Remove extraneous f prefix
(F541)
28-28: f-string without any placeholders
Remove extraneous f prefix
(F541)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
__init__.py (4)
11-23: Consider adding pagination and configuration supportThe wallpaper fetching function could be enhanced with:
- Support for pagination to access more wallpapers
- Configurable parameters (e.g., page size, categories, NSFW filter)
- Settings for the skill to customize these parameters
def get_wallpapers(query: Optional[str] = None): url = "https://wallhaven.cc/api/v1/search" - params = {"sorting": "random"} + params = { + "sorting": "random", + "page": "1", + "purity": "100", # SFW only + "categories": "111", # general/anime/people + "per_page": "24" + } if query: params["q"] = query + # Update params with skill settings if available + if hasattr(self, 'settings'): + params.update(self.settings.get('wallhaven_params', {}))
34-35: Remove unnecessary f-string prefixesThe string literals don't contain any interpolation, so the f-string prefix is unnecessary.
- self.gui.register_handler(f'wallpaper.next', self.handle_next) - self.gui.register_handler(f'wallpaper.prev', self.handle_prev) + self.gui.register_handler('wallpaper.next', self.handle_next) + self.gui.register_handler('wallpaper.prev', self.handle_prev)🧰 Tools
🪛 Ruff
34-34: f-string without any placeholders
Remove extraneous
fprefix(F541)
35-35: f-string without any placeholders
Remove extraneous
fprefix(F541)
Line range hint
78-135: Add error handling and user feedback in intent handlersThe intent handlers should handle cases where
fetch_wallpapersreturns an empty string or fails.Example fix for one handler (apply similar pattern to others):
@intent_handler("wallpaper.random.intent") def handle_random_wallpaper(self, message): image = self.fetch_wallpapers() + if not image: + self.speak_dialog("wallpaper.error") + return self.change_wallpaper(image) self.speak_dialog("wallpaper.changed")Would you like me to:
- Generate the error dialog file with appropriate responses?
- Apply this pattern to all intent handlers?
Line range hint
106-135: Add boundary checks before accessing picture_listThe navigation handlers should verify that
picture_listis not empty before attempting to access it.def handle_next(self, message=None): + if not self.picture_list: + self.speak_dialog("no.pictures") + return total = len(self.picture_list) self.pic_idx += 1 if self.pic_idx >= total:Apply similar checks to
handle_prevandhandle_set_wallpapermethods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
__init__.py(4 hunks)
🧰 Additional context used
🪛 Ruff
__init__.py
34-34: f-string without any placeholders
Remove extraneous f prefix
(F541)
35-35: f-string without any placeholders
Remove extraneous f prefix
(F541)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
__init__.py (1)
Line range hint
98-145: Consider consolidating duplicate image handling patternsThe intent handlers share similar patterns for handling images. Consider extracting common functionality into helper methods to reduce code duplication.
class WallpapersSkill(OVOSSkill): + def _handle_image_display(self, image, as_wallpaper=False): + if as_wallpaper: + self.change_wallpaper(image) + self.speak_dialog("wallpaper.changed") + else: + self.gui.show_image(image) + def handle_wallpaper_about(self, message): query = message.data["query"] self.speak_dialog("searching", {"query": query}) image = self.fetch_wallpapers(query) - self.change_wallpaper(image) - self.speak_dialog("wallpaper.changed") + self._handle_image_display(image, as_wallpaper=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
__init__.py(4 hunks)
🔇 Additional comments (5)
__init__.py (5)
44-48: LGTM! Clean initialization aligned with event-driven architecture
The initialization is well-structured with clear state tracking and PHAL registration.
50-54: Previous review comment about empty list handling still applies
Line range hint 57-78: LGTM! Well-implemented PHAL integration
The implementation properly follows the event-driven architecture for wallpaper management, with clear separation of concerns between collection management and wallpaper setting.
81-85: Previous review comment about redundant homescreen event still applies
88-97: Previous review comment about PHAL feedback handling still applies
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
__init__.py (2)
Line range hint
45-73: Add error handling and prevent race conditions in PHAL registrationThe PHAL registration methods could benefit from:
- Error handling for failed wallpaper fetches
- Preventing concurrent fetches
- Adding retry logic for failed registrations
Consider implementing:
def initialize(self): # state trackers self.pic_idx = 0 self.picture_list = [] + self._fetch_lock = threading.Lock() self.register_with_PHAL() def register_with_PHAL(self): + max_retries = 3 + retry_delay = 1 + + for attempt in range(max_retries): + try: self.bus.emit(Message("ovos.wallpaper.manager.register.provider", {"provider_name": self.skill_id, "provider_display_name": self.name})) self.bus.on(f"{self.skill_id}.get.wallpaper.collection", self.handle_wallpaper_scan) self.bus.on(f"{self.skill_id}.get.new.wallpaper", self.handle_wallpaper_get) - self.fetch_wallpapers() + if not self._safe_fetch_wallpapers(): + continue self.bus.emit(Message("ovos.wallpaper.manager.collect.collection.response", {"provider_name": self.skill_id, "wallpaper_collection": self.picture_list})) + return + except Exception as e: + LOG.error(f"PHAL registration attempt {attempt + 1} failed: {e}") + time.sleep(retry_delay) + LOG.error("Failed to register with PHAL after all retries") +def _safe_fetch_wallpapers(self, query=None) -> Optional[str]: + with self._fetch_lock: + try: + return self.fetch_wallpapers(query) + except Exception as e: + LOG.error(f"Failed to fetch wallpapers: {e}") + return None
Line range hint
115-146: Improve state handling in slideshow navigationThe slideshow navigation handlers need:
- Validation of picture_list state
- Protection against concurrent modifications
- Proper bounds checking
Consider this safer implementation:
+def _validate_slideshow_state(self): + """Validate slideshow state and return if it's valid""" + if not self.picture_list: + self.speak_dialog("no.pictures.available") + return False + if not 0 <= self.pic_idx < len(self.picture_list): + self.pic_idx = 0 + return False + return True @intent_handler(IntentBuilder("NextPictureIntent") .require("next").optionally("picture") .require("SlideShow")) def handle_next(self, message=None): + with self._fetch_lock: # Prevent concurrent modifications + if not self._validate_slideshow_state(): + return total = len(self.picture_list) self.pic_idx += 1 if self.pic_idx >= total: self.pic_idx = total - 1 self.speak_dialog("no.more.pictures") else: image = self.picture_list[self.pic_idx] self.gui.show_image(image) @intent_handler(IntentBuilder("PrevPictureIntent") .require("previous").optionally("picture") .require("SlideShow")) def handle_prev(self, message=None): + with self._fetch_lock: # Prevent concurrent modifications + if not self._validate_slideshow_state(): + return self.pic_idx -= 1 if self.pic_idx < 0: self.pic_idx = 0 self.speak_dialog("no.more.pictures") else: image = self.picture_list[self.pic_idx] self.gui.show_image(image) @intent_handler(IntentBuilder("MakeWallpaperIntent") .require("set").require("wallpapers") .optionally("picture") .require("SlideShow")) def handle_set_wallpaper(self, message=None): + with self._fetch_lock: # Prevent concurrent modifications + if not self._validate_slideshow_state(): + return image = self.picture_list[self.pic_idx] self.change_wallpaper(image) self.speak_dialog("wallpaper.changed")
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
README.md (2)
3-3: Format the URL as a proper markdown linkConvert the bare URL to a proper markdown link for better accessibility.
-Wallpapers from https://wallhaven.cc +Wallpapers from [wallhaven.cc](https://wallhaven.cc)🧰 Tools
🪛 Markdownlint
3-3: null
Bare URL used(MD034, no-bare-urls)
5-5: Enhance documentation with architectural detailsThe documentation should reflect the new event-driven approach mentioned in the PR objectives. Consider adding information about how the wallpaper changes are handled through events.
-> wallpapers are saved to `~/.local/share/wallpapers/` +> Wallpapers are downloaded to `~/.local/share/wallpapers/` and changes are handled through system events for better integration with the desktop environment.__init__.py (2)
13-41: Optimize HTTP requests by reusing sessionsThe function creates a new connection for each request. For better performance and to avoid connection overhead, consider using a session object:
+from requests.sessions import Session + +_session: Optional[Session] = None + +def _get_session() -> Session: + global _session + if _session is None: + _session = Session() + return _session + def get_wallpapers(query: Optional[str] = None, cache=True, max_pics: int = 5): url = "https://wallhaven.cc/api/v1/search" params = {"sorting": "random", "categories": "100"} if query: params["q"] = query try: - response = requests.get(url, params=params, timeout=10) + response = _get_session().get(url, params=params, timeout=10) response.raise_for_status() data = response.json()["data"] except (requests.RequestException, KeyError, ValueError) as e: LOG.error(f"Error fetching wallpapers: {str(e)}") return [] urls = [w["path"] for w in data][:max_pics] if cache: paths = [] local_wallpaper_storage = os.path.abspath(os.path.join(xdg_data_home(), "wallpapers")) os.makedirs(local_wallpaper_storage, exist_ok=True) for u in urls: LOG.debug(f"Downloading wallpaper: {u}") - pic = requests.get(u).content + pic = _get_session().get(u, timeout=10).content
92-95: Standardize dialog usage across intent handlersThe intent handlers have inconsistent dialog patterns. Some use "searching_random" while others use "searching". Standardize the dialog usage:
def handle_random_wallpaper(self, message): - self.speak_dialog("searching_random") + self.speak_dialog("searching", {"query": "random"}) image = self.fetch_wallpapers() self.change_wallpaper(image) self.speak_dialog("wallpaper.changed") def handle_random_picture(self, message=None): - self.speak_dialog("searching_random") + self.speak_dialog("searching", {"query": "random"}) image = self.fetch_wallpapers() self.gui.show_image(image)Also applies to: 99-101, 107-109, 115-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
README.md(1 hunks)__init__.py(4 hunks)locale/en-us/searching_random.dialog(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- locale/en-us/searching_random.dialog
🧰 Additional context used
🪛 Markdownlint
README.md
3-3: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
README.md (1)
Line range hint 12-17: Verify examples compatibility
Let's verify that these voice commands are still valid with the new implementation.
✅ Verification successful
Let me gather more specific information about the intent handlers and their implementation.
Let me check the intent files to see how these commands are mapped.
All example commands in README.md are supported by the implementation
Based on the intent files and handlers found in the codebase:
- "new wallpaper" matches
wallpaper.random.intentpattern "(change|random|new) (current|homescreen|the|) (wallpaper|wall paper)" - "change wallpaper to nature" matches
wallpaper.about.intentpattern "(change|random|new) (wallpaper|wall paper) (about|for|with|to) {query}" - "show me a picture" matches
picture.random.intentpattern "(show|display) me a (picture|photo|image)" - "show me a picture with dogs" matches
picture.about.intentpattern "(show|display) me a (picture|photo|image) (about|for|with) {query}" - "make that my wallpaper" is handled by
MakeWallpaperIntentwhich requires "set" and "wallpapers" keywords (implemented in the vocab files)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the intent handlers corresponding to these example commands
# in the Python files to ensure they're still supported
# Look for intent handlers that would handle these commands
rg -l "intent_handler|intent_file" --type py
# Search for specific keywords from the examples to ensure they're handled
rg -i "new wallpaper|change wallpaper|show.*picture|make.*wallpaper" --type py
Length of output: 280
Script:
#!/bin/bash
# Get the full context of the intent handlers and their implementation
rg -A 10 "intent_handler" __init__.py
# Also search for any vocab files that might define these intents
fd -e voc -e vocab
Length of output: 10098
Script:
#!/bin/bash
# Check the intent files that map to the handlers
fd -e intent
cat locale/en-us/*.intent 2>/dev/null
Length of output: 1129
🧰 Tools
🪛 Markdownlint
3-3: null
Bare URL used
(MD034, no-bare-urls)
1-1: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
7-7: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
8-8: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
dont try to set the wallpaper in the skill, just emit the proper event
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores